-
Notifications
You must be signed in to change notification settings - Fork 605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(meta): do not store upstream actors in merge node #20222
Conversation
f490830
to
58bcf47
Compare
This pull request has been modified. If you want me to regenerate unit test for any of the files related, please find the file in "Files Changed" tab and add a comment |
58bcf47
to
0e9e387
Compare
0e9e387
to
53af608
Compare
16121c8
to
9946e87
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
upstream_dispatcher_type: DispatcherType::Hash as _, | ||
fields: sink_fields.to_vec(), | ||
**merge_node = { | ||
#[expect(deprecated)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to move the attributes onto the field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had a try, but the attribute doesn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
8a9dab5
to
fc55c3f
Compare
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Previously we store the upstream actors information of each actor in the
upstream_actor_id
ofNodeBody::Merge
ofStreamNode
of eachStreamActor
. This is the only difference ofStreamNode
for each actor in a same fragment. In this PR, we will change to store the upstream actor information in a separate field besidesStreamActor
.We define type alias
In code of meta node, most appearance of
StreamActor
will be replaced withStreamActorWithUpstreams
, so that the upstreams information can be carried together with usage to originalStreamActor
. The fieldupstream_actor_id
ofMergeNode
andStreamActor
is marked deprecated in the proto definition, so that we can review all the original on the fields, to ensure the refactor logic is correct.Checklist
Documentation
Release note